Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kvserver: deflake TestRequestsOnFollowerWithNonLiveLeaseholder #107442

Merged

Conversation

tbg
Copy link
Member

@tbg tbg commented Jul 24, 2023

The test previously relied on aggressive liveness heartbeat expirations to
avoid running for too long. As a result, it was flaky since liveness wasn't
reliably pinned in the way the test wanted.

The hybrid manual clock allows time to jump forward at an opportune moment.
Use it here to avoid running with a tight lease interval.

On my gceworker, previously flaked within a few minutes. As of this commit, I
ran it for double-digit minutes without issue.

Fixes #107200.

Epic: None
Release note: None

tbg added 6 commits July 24, 2023 15:05
They're too verbose and the metrics will already make it clear when things
aren't set up properly.
This can be triggered rapidly because each replica might call this as it tries
and fails to acquire a lease.
The `Inc` is the blocking part, so log before.
…eLeaseholder

The test previously relied on aggressive liveness heartbeat expirations to
avoid running for too long. As a result, it was flaky since liveness wasn't
reliably pinned in the way the test wanted.

The hybrid manual clock allows time to jump forward at an opportune moment.
Use it here to avoid running with a tight lease interval.

On my gceworker, previously flaked within a few minutes. As of this commit, I
ran it for double-digit minutes without issue.

Fixes cockroachdb#107200.

Epic: None
Release note: None
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@tbg tbg requested a review from erikgrinaker July 25, 2023 10:27
@tbg tbg marked this pull request as ready for review July 25, 2023 10:27
@tbg tbg requested a review from a team July 25, 2023 10:27
@tbg tbg requested review from a team as code owners July 25, 2023 10:27
@tbg tbg added the db-cy-23 label Jul 25, 2023
Copy link
Contributor

@erikgrinaker erikgrinaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we're here, mind changing the test to use t.Logf instead of log.Infof()?

pkg/kv/kvserver/replica_range_lease.go Show resolved Hide resolved
@tbg
Copy link
Member Author

tbg commented Jul 26, 2023

While we're here, mind changing the test to use t.Logf instead of log.Infof()?

It was actually useful to have this in the logs, since then it interleaves with the actual logs. Otherwise, you don't get timestamps and you're left wondering about the relative ordering.

TFTR!

bors r=erikgrinaker

@craig
Copy link
Contributor

craig bot commented Jul 26, 2023

Build succeeded:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kv/kvserver: TestRequestsOnFollowerWithNonLiveLeaseholder failed
3 participants